[ENH] V1 -> V2 Migration - Flows (module) #1609
Conversation
Omswastik-11
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1609 +/- ##
==========================================
- Coverage 53.02% 51.57% -1.46%
==========================================
Files 36 47 +11
Lines 4326 4640 +314
==========================================
+ Hits 2294 2393 +99
- Misses 2032 2247 +215 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| class FlowsAPI(ResourceAPI, ABC): | ||
| @abstractmethod | ||
| def get( | ||
| self, | ||
| flow_id: int, | ||
| *, | ||
| return_response: bool = False, | ||
| ) -> OpenMLFlow | tuple[OpenMLFlow, Response]: ... |
There was a problem hiding this comment.
don't use return_response, see the other comment
| @abstractmethod | ||
| def list_page( | ||
| self, | ||
| *, | ||
| limit: int | None = None, | ||
| offset: int | None = None, | ||
| tag: str | None = None, | ||
| uploader: str | None = None, | ||
| ) -> pd.DataFrame: ... |
There was a problem hiding this comment.
let's call this method list instead of list_page to match other resources
| def list_page( | ||
| self, | ||
| *, | ||
| limit: int | None = None, | ||
| offset: int | None = None, | ||
| tag: str | None = None, | ||
| uploader: str | None = None, | ||
| ) -> pd.DataFrame: |
There was a problem hiding this comment.
nice, it can be refactored with private helper methods in class
| @staticmethod | ||
| def _convert_v2_to_v1_format(v2_json: dict[str, Any]) -> dict[str, dict]: |
There was a problem hiding this comment.
not a bad idea, we can discuss this, but where is this being used in sdk?
| from openml._api import api_context | ||
|
|
||
| xml_file = ( | ||
| openml.utils._create_cache_directory_for_id(FLOWS_CACHE_DIR_NAME, flow_id) / "flow.xml" | ||
| ) | ||
| flow_xml = openml._api_calls._perform_api_call("flow/%d" % flow_id, request_method="get") | ||
| result = api_context.backend.flows.get(flow_id, return_response=True) | ||
|
|
||
| with xml_file.open("w", encoding="utf8") as fh: | ||
| fh.write(flow_xml) | ||
| if isinstance(result, tuple): | ||
| flow, response = result | ||
| with xml_file.open("w", encoding="utf8") as fh: | ||
| fh.write(response.text) | ||
| else: | ||
| flow = result | ||
|
|
||
| return _create_flow_from_xml(flow_xml) | ||
| return flow |
There was a problem hiding this comment.
don't use return_response here, just remove all the caching related stuff which should leave the function with probably one-line code that calls the api_context.backend.flows.get method
| offset=offset, | ||
| tag=kwargs.get("tag"), | ||
| uploader=kwargs.get("uploader"), | ||
| ) |
There was a problem hiding this comment.
please use the suggested approach in #1575 (comment) for listing
| "post", | ||
| data={"name": name, "external_version": external_version}, | ||
| ) | ||
| from openml._api import api_context |
There was a problem hiding this comment.
import this on the top rather than in each function if that doesn't create any problem.
There was a problem hiding this comment.
why skipping these tests? are they because of changes in this PR?
There was a problem hiding this comment.
very nice! I'll into this in more details but looks good overall.
4 participants
Fixes #1601
added a
Createmethod inFlowAPIfor publishing flow but not refactored with oldpublish. (Needs discussion on this)Added tests using
fake_methodsso that we can test without localV2server . I have tested theFlowsV2methods (getandexists) anddeleteandlistwere not implemented inV2server so I skipped them .